-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch Font Awesome Icons from Web Font to SVG #2395
base: master
Are you sure you want to change the base?
Conversation
Lines 54 to 55 in 079d1a5
This should be removed from the Makefile now that the file is no longer being used. |
Also I just added one more icon to the header (fa-bolt) for image preloading. |
I've made the changes to Even doing Edit: I think I've just deleted all these changes with that |
What error is it giving you? |
I must have done something with that I was getting errors like I'm trying to revert it back to how it was yesterday night, but the Icons folder in my local copy is not there like it was... |
|
I think my issue was that I never actually added those SVGs with So now that I've done something in my attempt to fix what wasn't broken, I've lost them and will have to redo all those SVGs. |
Would be good to have a script that does them anyway, if just to document the process. I usually put stuff like that in |
Here it is as a gist then, since I've ruined this PR. I agree, a tool to do this would be awesome. I did it manually because I'm stupid. I'll be honest, got no clue how to go about making it. I mean, I think I get the theory behind it:
That might take my small brain a little while to process. |
Can't you just copy the SVGs out of the .user.js file if you still have it? |
I did that (have to remove the escaping \ characters), but as you said, a tool that did all the processing during the build process would make a lot more sense than doing that 5 steps manually. Because say for example the bolt icon, I had to go to that GitHub repo, get the black bolt.svg, add the attributes, run it through the viewBox thing, then run it through a GUI for svgo, then manually open |
Okay, let's pretend that didn't take me over 2 hours to solve. |
And if nothing else, perhaps as mentioned in the original issue, fontello could always be used to include a webfont of only the required icons, which would only reduce the size of the blob and leave everything else unchanged. |
In #1473 you linked a good summary of the advantages of SVGs over icon fonts: The main disadvantage of SVGs as far as 4chan X is concerned is that you can't easily replace them with CSS (you could use background images but you lose the ability to style the SVGs). But even if we switch to SVG, it would be possible to replace them with an icon font by setting the SVG to |
It could add bloat, but what about another section in the settings where it's a table of the icons on the left, and the SVG string on the right. That would give people the ultimate control - if someone wants FA4.7, then that's the default. If people want FA5 or Material Icons or the new Bootstrap icons, or literally any SVG icon, they could do that too. Then through Custom CSS they would be able to customise whatever needed to be changed (size, colours). |
I thought about that, but it would break a security policy of mine. User settings could potentially be tainted by a malicious site or a site with an XSS exploit, so I don't allow anything in user settings to execute JavaScript without a user action triggering it (e.g. clicking a link). |
Is this article of any relevance/help? I think I get what you're saying. Where it would store something in the JSON file like
and then when 4chan X is building the page, when it wants to add the eye icon, it checks the settings' JSON file to grab the appropriate SVG. But if someone has modified that file, it would execute arbitrary JS code. Right? What if 4chan X only stored certain attributes and their values? So, for example you could toggle By then though I guess it defeats the purpose of making it easier for the user anyway. Looks like best advice is to avoid user-uploaded SVGs, because there's lots of ways around it. If there's anything you'd like me to do with this PR let me know. I agree with you about placing another |
Since the plan is to go ahead with this, shall I add your proposed wiki entry to the 4chan X wiki? (Or you could add it; the wiki should be open for anyone to edit.)
It's relevant, but it doesn't solve the problem. I considered using SVGs in
More or less. I should clarify that a website or XSS exploit is unlikely to be able to modify files directly, but if 4chan X is running on the site, it could open the settings panel and simulate user input. Cleanup would be difficult, and a well-crafted exploit could modify the user's view of the settings to hide the infection. A notable example of an attack along these lines in the wild was the 2015 Imgur/8chan breach, which persistified itself using 8chan's setting to display a user's favorite boards in the header.
This would work, but I agree that it would not be user-friendly. The best UI I can imagine for this would reject many SVG files with a reason not intelligible to most users.
Good to hear. You say "another"; is there a reason to keep the data- attribute on the child?
I think most of what's left is testing that I'll have to do myself. I'll let you know if I find any problems I don't have an immediate fix for. |
I'll have another pass of the entry closer to a potential merge in case there are some changes made.
One could argue (for the sake of arguing, not that I agree/disagree) if a user wanted to change their icons to different SVGs, first they'd have to know that it was an option in the settings, and second what an SVG is and potentially some of the features of them. If there's a note saying "you can only use x, y, and z" with a live preview of the icon on the left to see if it worked or not, then that person would be willing to confine to those restrictions.
True. If you want to target the SVG, then
Sounds good. |
On the svgicons branch I've pulled in the changes from this pull request and written a little script tools/icons.js to automate the conversion process. You call it as |
Damn, that is fancy 😮! So that's what I was trying to convey in my head, but you know, what it actually would look like. Great work! |
…#2395 Also fix alt text not showing up for Catalog Settings button.
I feel I should mention, if you want to change the class naming convention, by all means go for it. I must have been in more of a BEM-esque mood when I wrote them. ninja edit Only because you have |
I think the clipping to bounding boxes is messing up the intended size relationships between the icons. It's most visible with the close icon, but you can see in the header shortcut icons how they look more inconsistent than before. Trying to fit then fit the trimmed SVGs into squares is contributing to the problem; this is especially visible with the gallery icon, which looks smaller than it should compared to the others. I'm going to try setting the viewbox differently and see if I can't restore the original look. |
That may also go back to what I’ve said in the previous issue where 4.7 icons aren’t consistent in their height/width. You’re right about the gallery icon being much bigger than nearly all the icons, which also mucks up visual consistency (plus the whole square is one size but a circle is bigger due to human eye playing tricks thing). I’m on my phone now, but I included a Codepen either here or in the other issue which had all the icons together which showcased the inconsistency of the sizes in 4.7. |
I've removed the clipping and made a few other adjustments: |
Oh no, what have I done... |
I'm trying to edit: how i did it I wanted to try and pull/update my local version so I could test this. Not sure if using negative margins to position elements/SVGs is the best way to go about it, given that things were okay with height/width being set. I had icons at different sizes (QR icons, menu button, header icons primarily) because it visually looked the same from master to SVG, so that the average user would (/should) not have noticed any change. |
Just remove those last two commits ( |
It really didn't; the close icons in particular were way too large. I'll try some other stuff, though. Thinking about trying trimming but putting explicit pixel width and height on each SVG so they display at a reasonable size by default. |
done in c9ab48d |
Ok, so, another silly question. I just got it working again on my PC. I now want to use your latest changes. I do In the mean time, I made this: Visually, the close and angle-down icon are going to cause the most strife, and perhaps those 2 can be treated differently to the rest. I think the other icons are now corrected, and should not have to be changed to look like they were. Personally, I think all the icons being the same (as in, generic class controls the size in CSS - 13px) is the most consistent and simplest. Worrying about vertical alignment and margins seems excessive, and adds extra rules that existing userstyles would either need to cancel out or rewrite anyway. Actually, ignore me. I don't know what I'm talking about. I see glaring issues with my method of center aligning the icons and 100% scaling them. I have a feeling the fact that |
https://developer.chrome.com/blog/new-in-chrome-105/#sanitizer-api Dead issue/PR I know, but with regards to stripping out unwanted HTML/malicious SVG code, I suppose something like the Sanitizer API could be used? |
This is the pull request following the discussion in #1473.
The
commit
s themselves are not as descriptive as they should be, I apologise. Not sure if that is something that can changed.I think my linter in Atom removed some trailing spaces in some files as well.
TO-DO
Icon
object.fa-times